Skip to content

London|MAY_2025|FATMA_DEGIRMENCI|STRUCTRING_AND_TESTING_DATA| SPRINT 3 #640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

fatmaevin
Copy link

@fatmaevin fatmaevin commented Jul 5, 2025

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

@fatmaevin fatmaevin added the Needs Review Participant to add when requesting review label Jul 5, 2025
@cjyuan cjyuan added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Participant to add when requesting review labels Jul 15, 2025
@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jul 15, 2025
@fatmaevin fatmaevin added the Needs Review Participant to add when requesting review label Jul 16, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Changes look good. Well done.

image

Leaving responses directly in the comment threads makes tracking the discussion easier. You can try the approach in future PRs.
Here is a simplified version of best practices ChatGPT suggested for responding to inline comments in a pull request:

  • ✅ Reply to every comment – Let the reviewer know you saw it.
  • ✏️ Make the change if needed – Fix the code if the comment points out a real issue.
  • 🤔 Explain if you don't agree – If you think the code is fine, politely explain why.
  • ✅ Mark as resolved when done – Only mark comments resolved after you fix or respond.
  • 💬 Keep replies short and polite – Be respectful and to the point.
  • ⏱️ Respond soon – Don’t wait too long to reply.
  • 🧪 Test your changes – Make sure your fixes actually work.
  • 📍 Reply directly under the comment – This keeps the conversation easy to follow.

Comment on lines +11 to +31
test("append 'st' to numbers ending in 1, except those ending in 11", () => {
expect(getOrdinalNumber(1)).toEqual("1st");
expect(getOrdinalNumber(21)).toEqual("21st");
expect(getOrdinalNumber(131)).toEqual("131st");
});
test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
expect(getOrdinalNumber(2)).toEqual("2nd");
expect(getOrdinalNumber(22)).toEqual("22nd");
expect(getOrdinalNumber(132)).toEqual("132nd");
});
test("append 'rd' to numbers ending in 3, except those ending in 13", () => {
expect(getOrdinalNumber(3)).toEqual("3rd");
expect(getOrdinalNumber(33)).toEqual("33rd");
expect(getOrdinalNumber(133)).toEqual("133rd");
});
test("should return 'th' for numbers ending with 11,12 or 13 ", () => {
expect(getOrdinalNumber(11)).toEqual("11th");
expect(getOrdinalNumber(111)).toEqual("111th");
expect(getOrdinalNumber(313)).toEqual("313th");
expect(getOrdinalNumber(512)).toEqual("512th");
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about numbers ending in 0, 4-9?

@cjyuan cjyuan added Complete Volunteer to add when work is complete and review comments have been addressed and removed Needs Review Participant to add when requesting review Reviewed Volunteer to add when completing a review labels Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and review comments have been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants